Drop step-specific "base Click command" implementations - #4457
Conversation
happz
added
code | cli
8af29bc to
b5373c7
Compare
b5373c7 to
d9fc940
Compare
d9fc940 to
d76bcd0
Compare
1268e00 to
48e6368
Compare
48e6368 to
3e49a3b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
thrix
left a comment
thrix
left a comment
There was a problem hiding this comment.
Code Review: Drop step-specific "base Click command" implementations
Reviewed against commit 3e49a3b.
Summary
Clean refactoring that consolidates 7 near-identical base_command() implementations (one per step plugin class) into a single shared implementation in BasePlugin. Removes ~220 lines of boilerplate. The shared version uses a new _step_class ClassVar backlink to derive the step name, and enhances --how to show available plugins as choices in the metavar.
Issues
1. Execute step gains PHASE_OPTIONS — functional change (medium)
The shared base_command() always applies @tmt.steps.PHASE_OPTIONS. However, the original execute step was the only step (out of 7) that did NOT use PHASE_OPTIONS. This means execute now gains --insert, --update, --update-missing, --allowed-how, --name, and --order CLI options it didn't have before. This contradicts the code | no functional change label. See inline comment.
2. Unrelated FileSystemPath/Package re-exports in tmt/steps/provision/__init__.py (minor)
These imports from tmt.package_managers are added as re-exports but nothing in the codebase imports them from tmt.steps.provision. Unrelated to the base_command refactoring. See inline comment.
Observations (no action needed)
- The
--howhelp text is now generic (f'Use specified method for {step_name} phase.') instead of step-specific (e.g.,'Use specified method for environment preparation.'). Minor wording change. - The
--howmetavar now lists available plugins (e.g.,beaker|connect|container|local|virtual) instead of the genericMETHOD. Nice improvement. - Removal of
# type: ignore[type-abstract]inplan.pyis correct —base_commandwas the only remaining abstract method in these plugin base classes, making them non-abstract now. - The module-level
XPlugin._step_class = Xassignments are well-documented and all 7 steps are covered.
Verdict
The refactoring is clean and correct. The main concern is issue #1 — the execute step gaining new CLI options should be acknowledged (and the no functional change label reconsidered) or handled explicitly.
Generated-by: Claude Code
3e49a3b to
5ae4d19
Compare
psss
left a comment
psss
left a comment
There was a problem hiding this comment.
Nice deduplication! Just one wording nitpick.
dfac67b to
67d9dc6
Compare
67d9dc6 to
07c9e82
Compare
They are not that different, each step class just used different step name, and that was it. Dropping the `base_command` method from `Step`-like classes, replacing them with a shared implementation. Also, `--how` now prints allowed/known plugins as possible choices. I was editing the help text, and this felt like a natural change of the option.
07c9e82 to
fe7aa81
Compare
5 participants
They are not that different, each step class just used different step name, and that was it. Dropping the
base_commandmethod fromStep-like classes, replacing them with a shared implementation.Also,
--hownow prints allowed/known plugins as possible choices. I was editing the help text, and this felt like a natural change of the option.Pull Request Checklist